Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: wallet_tests.cpp #180

Merged
merged 1 commit into from Mar 14, 2024
Merged

Fix: wallet_tests.cpp #180

merged 1 commit into from Mar 14, 2024

Conversation

ghost
Copy link

@ghost ghost commented Feb 29, 2024

Value 1 and 2 on Line 551, Line 567, Line 574, Line 596 are exactly the same in the bitcoin 22 original file. And what i saw in DigiByte 7.17.3.

https://github.com/DigiByte-Core/digibyte/blob/release/v7.17.3/src/wallet/test/wallet_tests.cpp

wallettests


1). wallet/test/wallet_tests.cpp(551)
2). wallet/test/wallet_tests.cpp(558)
3). wallet/test/wallet_tests.cpp(571)
4). wallet/test/wallet_tests.cpp(578)
5). wallet/test/wallet_tests.cpp(600)

@ghost
Copy link
Author

ghost commented Feb 29, 2024

Normally do not change a test. but this test fails because it is modified.
Also COINBASE_MATURITY_2 - COINBASE_MATURITY + 1 are not in dgb 7.17.3 ( wallet_tests.cpp ) and not in the original wallet_tests.cpp btc 22 file.

https://github.com/DigiByte-Core/digibyte/pull/31/files
https://github.com/DigiByte-Core/digibyte/pull/32/files
( Cannot find all file that change wallet_tests.cpp )

BTC 22
btc22

https://bitcoincore.org/bin/bitcoin-core-22.0/

@gto90 gto90 assigned ghost Mar 1, 2024
@gto90 gto90 added the bug Something isn't working label Mar 1, 2024
@gto90
Copy link
Member

gto90 commented Mar 1, 2024

@Jongjan88 , thank you for the additional context. I do see that the test was changed in PR #62 . However, @SmartArray had made this change in PR #62, which was merged quite a while ago.

@JaredTate or @SmartArray can you provide some feedback on this PR and what changed in the protocol that causes the changes introduced by #62 to fail now?

I am not opposed to approving this if it resolves the test failure but I am first trying to understand the reason @SmartArray had made this change, why that allowed the test to pass for RC1, and why the test is now failing.

@ghost
Copy link
Author

ghost commented Mar 1, 2024

@gto90 TY I could not find it. in which file it was modified. And of course I didn't adjust it to Digibyte Core. But change it to the original values.

Screenshot from 2024-03-01 19-37-34

@gto90
Copy link
Member

gto90 commented Mar 1, 2024

@gto90 TY I could not find it. in which file it was modified. And of course I didn't adjust it to Digibyte Core. But change it to the original values.

Screenshot from 2024-03-01 19-37-34

I totally understand @Jongjan88 , I am just hoping to get some feedback from @SmartArray and/or @JaredTate to make sure we don't have an issue in the protocol as I am quite sure @SmartArray had a very valid reason for making that change.

@ghost
Copy link
Author

ghost commented Mar 12, 2024

@Jongjan88 , thank you for the additional context. I do see that the test was changed in PR #62 . However, @SmartArray had made this change in PR #62, which was merged quite a while ago.

@JaredTate or @SmartArray can you provide some feedback on this PR and what changed in the protocol that causes the changes introduced by #62 to fail now?

I am not opposed to approving this if it resolves the test failure but I am first trying to understand the reason @SmartArray had made this change, why that allowed the test to pass for RC1, and why the test is now failing.

gto90: "why that allowed the test to pass for RC1, and why the test is now failing."

All the 70/80 python test that don't work with rc3 pass for rc2. @ycagel @gto90 @JaredTate i hope we can review this PR so i can delete the branch. For more info look at pr #62. Where this test is changed to work with rc1.

@JaredTate
Copy link

So it compiles and runs... but Make Check fails long before it ever gets to this file for me. So I am not exactly sure how you are running it and testing it?

@ghost
Copy link
Author

ghost commented Mar 14, 2024

So it compiles and runs... but Make Check fails long before it ever gets to this file for me. So I am not exactly sure how you are running it and testing it?

  ./autogen.sh
  ./configure
  make -j 8
  make check
cd src/test
./test_digibyte --run_test=wallet_tests --log_level=message

@JaredTate JaredTate mentioned this pull request Mar 14, 2024
@JaredTate
Copy link

./test_digibyte --run_test=wallet_tests --log_level=message

Perfect, that makes sense. I fixed the main folder unit tests, so now make check fails on this one for me. So testing this now.

Copy link

@JaredTate JaredTate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK! Great work! Now hopefully Make check works totally!

Screenshot 2024-03-14 at 3 39 03 PM

Copy link
Member

@ycagel ycagel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cACK

Copy link
Member

@gto90 gto90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@ycagel ycagel merged commit a5c305f into DigiByte-Core:develop Mar 14, 2024
1 of 2 checks passed
@ghost ghost deleted the testwallet branch March 14, 2024 21:52
@DigiByte-Core DigiByte-Core deleted a comment from Gideaos Mar 14, 2024
@JaredTate
Copy link

That idiot is posting phishing links. Guess they can spam github with crap now too.

@DigiByte-Core DigiByte-Core deleted a comment from Gideaos Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants